Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mail design #1446

Merged
merged 4 commits into from
Nov 30, 2016
Merged

Fix mail design #1446

merged 4 commits into from
Nov 30, 2016

Conversation

eppfel
Copy link
Member

@eppfel eppfel commented Sep 19, 2016

As in issue #1326

I imitate the nextcloud header.

I replced the gif with the png, but maybe some other apps use it.

bildschirmfoto 2016-09-19 um 15 57 55

@nextcloud/designers

Todo

  • typo "an nextlcoud"
  • fix the gif/png
  • header, just with a circled logo
  • bold username and link

@mention-bot
Copy link

@eppfel, thanks for your PR! By analyzing the annotation information on this pull request, we identified @schiessle, @rullzer and @scolebrook to be potential reviewers

@eppfel eppfel added 1. to develop Accepted and waiting to be taken care of design Design, UI, UX, etc. labels Sep 19, 2016
@MariusBluem MariusBluem added this to the Nextcloud 11.0 milestone Sep 19, 2016
@MorrisJobke
Copy link
Member

MorrisJobke commented Sep 19, 2016

I have found following project that aims for nice responsive emails: https://htmlemail.io/

maybe we lend some concepts from them ;)

Edit: https://mjml.io/ also looks nice

@jancborchardt
Copy link
Member

@eppfel what do you think of just using the blue logo there? That would look a lot nicer and simpler on white probably.

Btw there’s a typo in the mail: »an Nextcloud account«

@eppfel
Copy link
Member Author

eppfel commented Oct 18, 2016

@jancborchardt Yeah, blue logo would be more fitting, I guess. But that still leaves us with the theming question...

@jancborchardt
Copy link
Member

I guess just changing for the correct logo is easy with the dynamic icon creation by @juliushaertl #840 ;)

@juliushaertl
Copy link
Member

This should already be quite easy even without #840.

Just for the theming stuff, it would of course be simpler to keep the blue backgorund with the white logo, just as it is used everywhere else in the Nextcloud UI at the moment. Otherwise we would again introduce a second logo, that may need some extra handling in case of theming.

@eppfel
Copy link
Member Author

eppfel commented Oct 18, 2016

Just for the theming stuff, it would of course be simpler to keep the blue backgorund with the white logo, just as it is used everywhere else in the Nextcloud UI at the moment. Otherwise we would again introduce a second logo, that may need some extra handling in case of theming.

That was actually what I was referring to.

@eppfel eppfel self-assigned this Oct 18, 2016
@jancborchardt
Copy link
Member

Maybe then make it a blue circle, kinda like the stickers? That might look nicer than the rectangle which feels a bit misplaced.

@jancborchardt
Copy link
Member

@eppfel what do you think about the circle idea? We could center that icon too maybe.

And while we’re at improving the HTML mail, maybe also bold the username and the access link?

@eppfel
Copy link
Member Author

eppfel commented Nov 4, 2016

@jancborchardt Sounds all good ideas. I'm just super busy ATM.
If someone wants to hijack this one... feel free!

@jancborchardt jancborchardt added good first issue Small tasks with clear documentation about how and in which place you need to fix things in. help wanted labels Nov 4, 2016
@schiessle
Copy link
Member

schiessle commented Nov 9, 2016

@eppfel
Copy link
Member Author

eppfel commented Nov 28, 2016

I want to leave this as minor fix, instead of making it an overall Mail redesign. Because the state it is now, is not acceptable at all. We then even could look at backporting it.
And after the discussion from #2071 is finished, we can make a new PR for the redesign.

What do you think? @jancborchardt @MorrisJobke @schiessle

@MorrisJobke
Copy link
Member

I just rebased :)

@MorrisJobke
Copy link
Member

MorrisJobke commented Nov 29, 2016

I have no idea why, but the "shared a file with you" email(left) has a wrong URL ('/themes/` should not be in there) but the "new user" email works fine(right):

bildschirmfoto 2016-11-29 um 16 16 32

@eppfel: Any idea? And remind that I rebased and you need to fetch that version first.

cc @rullzer @juliushaertl for this

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Nov 29, 2016
@MorrisJobke
Copy link
Member

Ah found it - there is a new template ;)

@MorrisJobke
Copy link
Member

I just tested this and it works really nice 👍

@MorrisJobke
Copy link
Member

I removed the leftovers from #1929 - cc @rullzer

eppfel and others added 2 commits November 30, 2016 01:06
Signed-off-by: Felix A. Epp <work@felixepp.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@eppfel
Copy link
Member Author

eppfel commented Nov 30, 2016

@MorrisJobke Signed, rebased ❗️, but typo and bold username/url are still there. I do not thoroughly understand transifex, yet.

@codecov-io
Copy link

codecov-io commented Nov 30, 2016

Current coverage is 56.99% (diff: 100%)

Merging #1446 into master will increase coverage by 0.02%

@@             master      #1446   diff @@
==========================================
  Files          1191       1189     -2   
  Lines         72072      72037    -35   
  Methods        7299       7299          
  Messages          0          0          
  Branches       1210       1210          
==========================================
- Hits          41057      41056     -1   
+ Misses        31015      30981    -34   
  Partials          0          0          

Powered by Codecov. Last update 83cef4e...8e361b7

* a nextcloud
* Strong username & url

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member

rullzer commented Nov 30, 2016

@eppfel I fixed the missing todo's. Please double check.

Else good to go from my POV 👍

@@ -11,7 +11,7 @@
<td width="20px">&nbsp;</td>
<td style="font-weight:normal; font-size:0.8em; line-height:1.2em; font-family:verdana,'arial',sans;">
<?php
print_unescaped($l->t('Hey there,<br><br>just letting you know that you now have an %s account.<br><br>Your username: %s<br>Access it: <a href="%s">%s</a><br><br>', array($theme->getName(), $_['username'], $_['url'], $_['url'])));
print_unescaped($l->t('Hey there,<br><br>just letting you know that you now have a %s account.<br><br>Your username: <strong>%s</strong><br>Access it: <strong><a href="%s">%s</a></strong><br><br>', array($theme->getName(), $_['username'], $_['url'], $_['url'])));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of interest? How does transifex allocate the translations, if the key changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows a new untranslated string. but it does remember the original one and shows it in 'likely' matches or something. So that users have help in translating the new one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. So, it's fine changing these and not worry about the translation. Thanks for the clarification. ❤️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - just change it and it will show up after 3 am on transifex ;)

@MorrisJobke
Copy link
Member

👍

@MorrisJobke MorrisJobke merged commit 86168c8 into master Nov 30, 2016
@MorrisJobke MorrisJobke deleted the fix-mail-design branch November 30, 2016 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. good first issue Small tasks with clear documentation about how and in which place you need to fix things in. help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants